Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering rework #1408

Closed
wants to merge 7 commits into from
Closed

Rendering rework #1408

wants to merge 7 commits into from

Conversation

rom1v
Copy link
Collaborator

@rom1v rom1v commented May 18, 2020

I reworked many things related to the window/rendering.

Firstly, I improved the maximized/fullscreen handling to avoid weird behaviors on some platforms (see commit messages 6dc113e and eb189ce).

Then, (as a followup of #1307 that I closed because it was not ready) I reworked the rendering to position and scale the content (and the events) manually (i.e. without using the SDL "logical size" feature). (commit a6dcbf3)

It has several benefits:

  • it allows to avoid rounding inconsistencies between internal SDL logical size and the window size (causing 1 unexpected row/column of pixels blacks in the "optimal size")
  • it will make possible to draw visual feedback at the expected size (for Pinch zooming support #24 for example) in the future
  • above all, it fixes Touch input offset on secondary monitor #15 🎉

I am now quite confident that it works correctly.

But since it impacts heavily the rendering and the event forwarding (which must NOT be broken at all), more tests from users are welcome:

  • if you can, also test with a secondary screen
  • check that clicks are correctly applied at the right position
    • even if you resize so that there are black borders
    • both in fullscreen or maximized or normal state
    • with or without content rotation (Ctrl+ / Ctrl+)
  • try to maximize, then fullscreen (both Ctrl+f and "expand to fullscreen", which behaves differently), rotate device, then unfullscreen (check that the window is correctly restored in maximized state), then unmaximize (check that the window is correctly restored at the correct size).
  • use the app normally, you could find other edge cases :)

Binaries

To simplify testing, here are binaries.

For windows users, take both scrcpy.exe and scrcpy-server, and replace them in your scrcpy v1.13 release.

For other platforms, take scrcpy-server and build only the client.

  • scrcpy.exe
    SHA256: 7d61f31c8e33aa1b70a43b38b7c40c7e5289683a959e8426b34c3a488a385186
  • scrcpy-server
    SHA256: 6d124307e081b8bbccc478dcf82731aad3cc994e7502197c9bae3847d3169dc7

Thank you for your feedbacks 😉

rom1v added 7 commits May 11, 2020 03:12
When the content size changes, either on frame size or client rotation
changes, the window must be resized. Factorize for both cases.
If the content size changes (due to rotation for example) while the
window is maximized or fullscreen, the resize must be applied once
fullscreen and maximized are disabled.

The previous strategy consisted in storing the windowed size, computing
the target size on rotation, and applying it on window restoration. But
tracking the windowed size (while ignoring the non-windowed size) was
tricky, due to unspecified order of SDL events (e.g. size changes can be
notified before "maximized" events), race conditions when reading window
flags, different behaviors on different platforms...

To simplify the whole resize management, store the old content size (the
frame size, possibly rotated) when it changes while the window is
maximized or fullscreen, so that the new optimal size can be computed on
window restoration.
On Windows, in maximized+fullscreen state, disabling fullscreen mode
unexpectedly triggers the "restored" then "maximized" events, leaving
the window in a weird state (maximized according to the events, but not
maximized visually).

Moreover, apply_pending_resize() asserts that fullscreen is disabled.

To avoid the issue, if fullscreen is set, just ignore the "restored"
event.
In maximized state (but not fullscreen), it was possible to resize to
fit the device screen (with Ctrl+x or double-clicking on black borders).

This caused problems on macOS with the "expand to fullscreen" feature,
which behaves like a fullscreen mode but is seen as maximized by SDL.
In that state, resizing to fit causes unexpected results.

To keep the behavior consistent on all platforms, just disable "resize
to fit" when the window is maximized.
Extract the computation to detect whether the current size of the window
is already optimal.

This will allow to reuse it for rendering.
Position and scale the content "manually" instead of relying on the
renderer "logical size".

This avoids possible rounding differences between the computed window
size and the content size, causing one row or column of black pixels on
the bottom or on the right.

This also avoids HiDPI scale issues, by computing the scaling manually.

This will also enable to draw items at their expected size on the screen
(unscaled).

Fixes #15 <#15>
On macOS with renderer "metal", HiDPI scaling may be incorrect on
initialization when several displays are connected.

Resetting the window size fixes the problem.

Refs #15 <#15>
@carstenhag
Copy link

carstenhag commented May 19, 2020

Tried out all cases which you described. Seems like the issue is fixed on all of them!
In this one I wasn't sure what it was about though:

try to maximize, then fullscreen (both Ctrl+f and "expand to fullscreen", which behaves differently), rotate device, then unfullscreen (check that the window is correctly restored in maximized state), then unmaximize (check that the window is correctly restored at the correct size).

What did you mean with "expand to fullscreen"? I tried starting it with -f, I used the cmd+f shortcut, the yellow mac maximize button, all of those worked fine.

Edit: Green button, not yellow

@rom1v
Copy link
Collaborator Author

rom1v commented May 19, 2020

Tried out all cases which you described. Seems like the issue is fixed on all of them!

🎉

What did you mean with "expand to fullscreen"?

https://support.apple.com/fr-fr/guide/mac-help/mchl9c21d2be/mac

@carstenhag
Copy link

Yeah I did that, haha. All fine

@rom1v
Copy link
Collaborator Author

rom1v commented May 23, 2020

It seems to work as expected (I also did more tests and received private feedbacks).

So I just merged it into dev 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants